Flesh out tests for types.go, parser.go; add validation_test.go, update validation.go; add canonical formatting for cpu and memory resources #1
Conversation
There was a problem hiding this comment.
Very nice changes! Generally I like the updates.
Only part to be discussed and considered is the consistency in the internal representation. I'm not a huge fan of a structure having two public fields (i.e. CPU and CPURaw) that are supposed to be consistent with each other. This can lead to possible inconsistency. We should consider a way to only have one field that'd act as the source of truth and render other views as methods.
There was a problem hiding this comment.
I've more carefully stepped through the code and suggested more concrete changes to be made. Speficially, I'd prefer we stick to original value of supplied memory and CPU to be stored in Memory and CPU field, but provide getMemory and getCPU methods that would provide fully normalized numerical values.
I've also implemented a few improvments in the code to be made -- please refer to the comments and let me know if you have any questions!
| // Access SSH keys | ||
| sshKeys, err := cfg.GetSSHKeys() | ||
| if err != nil { | ||
| log.Fatal(err) |
There was a problem hiding this comment.
Actually log.Fatal logs the output to stderr and then call os.Exit(1) so return statement would be unreachable here.
| // canonical textual CPU quantity that k8s accepts, e.g. "2", "2.5", or "500m". | ||
| // It trims whitespace and lowercases the unit. It does NOT add "m" for you; | ||
| // integers/floats remain core-based unless the input already used "m". | ||
| func normalizeCPUText(v any) (string, error) { |
There was a problem hiding this comment.
I find normalizeCPUText to be a bit misleading -- maybe like normalizeToCPUText?
|
|
||
| func bytesToMi(bytes float64) (int64, error) { | ||
| if bytes < 0 || math.IsNaN(bytes) || math.IsInf(bytes, 0) { | ||
| return 0, fmt.Errorf("memory must be >= 0") |
There was a problem hiding this comment.
maybe slightly more descriptive error message to cover the IsNaN and IsInf case like memory must be valid value >= 0.0 (not NaN or Inf)?
| return roundFloatToInt64(miFloat) | ||
| } | ||
|
|
||
| func roundFloatToInt64(v float64) (int64, error) { |
There was a problem hiding this comment.
I know it's quite self-explanatory, but mind adding a very short function docstring to this and the one above?
| return f, true | ||
| } | ||
|
|
||
| // Case-insensitive suffix check. |
There was a problem hiding this comment.
Let's make this conform to Go docstring styles like hasSuffixFold checks if the string has the suffix, performing Unicode case folding for case-insensitive check
| } | ||
|
|
||
| // Memory returns the memory resource allocation as a string suitable for Kubernetes manifests. | ||
| // Memory returns "Gi" or "Mi" ("" means omit). |
There was a problem hiding this comment.
Slightly more description here would be nice -- something to the similar detail as for CPU()
internal/config/types.go
Outdated
| // It handles flexible input types from YAML (string, int, float64) and converts them | ||
| // to a consistent string format. | ||
| // CPU returns the canonical CPU quantity formatted for Kubernetes (e.g., "2500m"). | ||
| // Assumes Resources.CPU has already been normalized to millicores. |
There was a problem hiding this comment.
This is no longer true about requiring Resources.CPU to be normalized to millicores, right (you are using getCanonicalCPU)
2 participants
Breakdown of changes to each file:
parser.go
types.go
validation.go
parser_test.go
types_test.go
validation_test.go
renderer_test.go
statefulset.yaml
generate.go
example_test.go